Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove prometheus-metrics-shaded-protobuf dependency from the agent #12564

Closed
wants to merge 2 commits into from

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Nov 4, 2024

@trask WDYT?

@github-actions github-actions bot requested a review from theletterf November 4, 2024 12:38
@trask
Copy link
Member

trask commented Nov 4, 2024

I'm not sure we can do this, it looks like exposition format is controlled by the client scraper via the Accept http header:

https://github.com/prometheus/client_java/blob/fc583c2e5813d4ab510f3464f7bf5d2f9b4d3ce2/prometheus-metrics-exposition-formats/src/main/java/io/prometheus/metrics/expositionformats/ExpositionFormats.java#L34-L36

@laurit
Copy link
Contributor Author

laurit commented Nov 5, 2024

I'm not sure we can do this, it looks like exposition format is controlled by the client scraper via the Accept http header

Thanks. https://opentelemetry.io/docs/specs/otel/metrics/sdk_exporters/prometheus/ doesn't say that the protobuf format must be supported. As far as I can tell support for the protobuf format was added not too long ago with open-telemetry/opentelemetry-java#6015

@trask
Copy link
Member

trask commented Nov 5, 2024

added not too long ago with open-telemetry/opentelemetry-java#6015

this part is interesting:

In Prometheus, exponential histograms require the Prometheus protobuf exposition format.

@laurit
Copy link
Contributor Author

laurit commented Nov 5, 2024

Prometheus seems to support OTLP https://prometheus.io/docs/guides/opentelemetry/

@trask
Copy link
Member

trask commented Nov 6, 2024

@fstab any thoughts?

@fstab
Copy link
Member

fstab commented Nov 6, 2024

In Prometheus, exponential histograms require the Prometheus protobuf exposition format.

Yes, exponential histograms are only supported in Prometheus protobuf format, not in Prometheus text format. There's work underway to specify a text representation, but that's not implemented yet.

If a Prometheus scraper runs with exponential histogram support enabled, it will signal protobuf support in the HTTP Accept: header, and then the Prometheus exporter will expose protobuf format, including exponential histograms.

I don't think this should be removed, otherwise support for exponential histograms would be lost.

@fstab
Copy link
Member

fstab commented Nov 6, 2024

Also, as the protobuf dependency is shaded into the io.prometheus package there should not be a conflict with the protobuf version used by the OTel SDK itself.

@laurit
Copy link
Contributor Author

laurit commented Nov 6, 2024

I don't think this should be removed, otherwise support for exponential histograms would be lost.

I created this PR under the assumption that text format can represent everything. I'll close it as we don't wish to loose this functionality.

Also, as the protobuf dependency is shaded into the io.prometheus package there should not be a conflict with the protobuf version used by the OTel SDK itself.

Neither the otel sdk nor the agent depend on the protobuf lbrary. The size of protobuf library is bit less than 2mb which is roughly 9% of the agent size.

@laurit laurit closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants